Fix to avoid deleting non ScalarDB tables when dropping namespaces #3092

KodaiD · 2025-10-27T12:15:06Z

Description

This PR fixes a bug that drops non-ScalarDB-managed tables when dropping namespaces.

Suppose a user has multiple tables under the schema "ns" that are not managed by ScalarDB. If the user imports one of these tables using importTable, ScalarDB will only see one table existing within "ns". Subsequently, if the user drops that table through ScalarDB, ScalarDB will determine that "ns" is empty and internally issue a DROP SCHEMA ns query. As a result, all tables not under ScalarDB's management will be dropped.

Some databases, including MySQL, allow schema deletion to succeed even if tables still exist within the schema, and there is no way to prevent this. Therefore, it is necessary to check for the existence of tables before deleting the schema.

Since it is difficult to include table existence checking logic within RdbEngineStrategy.dropNamespaceSql, the table existence check is performed for all databases in the JdbcAdmin abstraction layer.

Related issues and/or PRs

N/A

Changes made

Updated JdbcAdmin to check if non-ScalarDB tables exist before dropping the specified schema.
Updated unit test cases related to dropNamespace.
Added an integration test case that covers the bug.

Checklist

I have commented my code, particularly in hard-to-understand areas.
I have updated the documentation to reflect the changes.
I have considered whether similar issues could occur in other products, components, or modules if this PR is for bug fixes.
Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
Tests (unit, integration, etc.) have been added for the changes.
My changes generate no new warnings.
Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

N/A

Release notes

N/A

KodaiD · 2025-10-27T12:15:20Z

/gemini review

gemini-code-assist

Code Review

This PR addresses a bug in JdbcAdmin that caused non-ScalarDB tables to be dropped when dropping namespaces. The fix involves checking for remaining tables in a namespace before dropping it and throwing an exception if non-ScalarDB tables exist. Unit and integration tests have been updated to reflect this change.

...t/src/main/java/com/scalar/db/api/DistributedStorageAdminImportTableIntegrationTestBase.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java

Copilot

Pull Request Overview

This PR fixes a critical bug where dropping a namespace in ScalarDB could inadvertently delete non-ScalarDB-managed tables. The fix adds a check to verify that no non-ScalarDB tables remain in a namespace before allowing the namespace to be dropped, throwing an IllegalStateException if any such tables are found.

Key Changes:

Added namespace emptiness validation in JdbcAdmin.dropNamespace() before attempting to drop the schema
Implemented getTableNamesInNamespaceSql() method across all RdbEngine implementations to query existing tables in a namespace
Added integration tests to verify the fix prevents deletion of non-ScalarDB tables

Reviewed Changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
`JdbcAdmin.java`	Added table existence check before dropping namespace
`RdbEngineStrategy.java`	Added interface method for querying table names in a namespace
`RdbEngineMysql.java`	Implemented SQL query to get table names for MySQL
`RdbEnginePostgresql.java`	Implemented SQL query to get table names for PostgreSQL
`RdbEngineSqlServer.java`	Implemented SQL query to get table names for SQL Server
`RdbEngineOracle.java`	Implemented SQL query to get table names for Oracle
`RdbEngineDb2.java`	Implemented SQL query to get table names for DB2
`RdbEngineSqlite.java`	Implemented SQL query to get table names for SQLite
`JdbcAdminTest.java`	Updated unit tests to mock the new namespace emptiness check
`DistributedStorageAdminImportTableIntegrationTestBase.java`	Added integration test and updated teardown logic
`DistributedTransactionAdminImportTableIntegrationTestBase.java`	Added integration test for transaction admin
`JdbcAdminImportTableIntegrationTest.java`	Implemented test utility method and disabled SQLite test
`ConsensusCommitAdminImportTableIntegrationTestWithJdbcDatabase.java`	Implemented test utility method and disabled SQLite test

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlite.java

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineSqlServer.java

core/src/main/java/com/scalar/db/storage/jdbc/RdbEnginePostgresql.java

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineOracle.java

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineMysql.java

core/src/main/java/com/scalar/db/storage/jdbc/RdbEngineDb2.java

...t/src/main/java/com/scalar/db/api/DistributedStorageAdminImportTableIntegrationTestBase.java

...c/main/java/com/scalar/db/api/DistributedTransactionAdminImportTableIntegrationTestBase.java

komamitsu

LGTM, thank you!

feeblefakie

Overall, looking good. Thank you for the prompt work.
But, should we also check other databases, such as Cassandra, Cosmos, and Dynamo?
Cassandra will probably drop a keyspace even if a table exists in the keyspace.

KodaiD · 2025-10-28T05:11:21Z

Overall, looking good. Thank you for the prompt work. But, should we also check other databases, such as Cassandra, Cosmos, and Dynamo? Cassandra will probably drop a keyspace even if a table exists in the keyspace.

@feeblefakie Ah, I see... I had assumed that the case where ScalarDB managed tables and unmanaged tables coexist only occurs when importTable is executed. Therefore, I only implemented the bug fix for JDBC databases. However, even in the case of non-JDBC databases, it's possible for a user to create a ScalarDB table and then create a non-ScalarDB managed table using the same namespace name. Let me check.

…when-dropping-namespaces

KodaiD · 2025-10-29T01:33:41Z

In Cassandra adapter, table metadata of both ScalarDB-managed tables and non-ScalarDB-managed tables is stored in the Cassandra metadata table. Therefore, Cassandra can detect remaining tables when dropping a key space, even if they're non-ScalarDB-managed tables.

For DynamoDB, since it doesn't have a concept of namespaces (a ScalarDB namespace is represented as a table name prefix), we don't need to care about the bug. SQLite is as well.

Therefore, a check for the existence of non-ScalarDB-managed tables has been added to CosmosAdmin and JdbcAdmin (except for SQLite).

...gration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java

KodaiD · 2025-10-29T01:47:00Z

@feeblefakie Now the PR is updated. PTAL!

brfrn169

Left several comments. PTAL!

core/integration.sqlite3

core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java

core/src/test/java/com/scalar/db/storage/cosmos/CosmosAdminTest.java

core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java

...t/src/main/java/com/scalar/db/api/DistributedStorageAdminImportTableIntegrationTestBase.java

brfrn169

Left a couple of comments. Other than that, LGTM! Thank you!

core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java

core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java

brfrn169

LGTM! Thank you!

feeblefakie

I might miss something, but Cassandra drops a keyspace even if there exists a table.
So, should it check the table existence for Cassandra as well to meet the following requirement?

dropNamespace won’t drop a given namespace if there is at least one table that is either ScalarDB-managed or non-ScalarDB-managed.

KodaiD · 2025-10-30T05:01:45Z

I might miss something, but Cassandra drops a keyspace even if there exists a table. So, should it check the table existence for Cassandra as well to meet the following requirement?
dropNamespace won’t drop a given namespace if there is at least one table that is either ScalarDB-managed or non-ScalarDB-managed.

@feeblefakie I appreciate the confirmation. Unlike other adapters, Cassandra adapter does not create its own table metadata table. Instead, it uses Cassandra's table metadata table. Cassandra's table metadata table stores information about both ScalarDB tables and non-ScalarDB tables. Therefore, before CassandraAdmin.dropNamespace is called, the following part of CommonDistributedStorageAdmin.dropNamespace can detect the existence of non-ScalarDB tables and throw an exception.

if (!getNamespaceTableNames(namespace).isEmpty()) {
  throw new IllegalArgumentException(
      CoreError.NAMESPACE_NOT_EMPTY.buildMessage(namespace, getNamespaceTableNames(namespace)));
}

The integration test case added in this PR also succeeds with Cassandra adapter, confirming that dropNamespace does not delete non-ScalarDB tables.

Torch3333

LGTM, thank you!

feeblefakie · 2025-10-30T07:36:55Z

@KodaiD Ah, OK. Thanks for the clarification!

feeblefakie

LGTM! Thank you!

KodaiD added 2 commits October 27, 2025 15:54

Add integration tests

2eb60eb

Fix to avoid deleting non ScalarDB tables when dropping namespaces

169e4b6

KodaiD self-assigned this Oct 27, 2025

KodaiD added the bugfix label Oct 27, 2025

gemini-code-assist bot reviewed Oct 27, 2025

View reviewed changes

...t/src/main/java/com/scalar/db/api/DistributedStorageAdminImportTableIntegrationTestBase.java Outdated Show resolved Hide resolved

core/src/main/java/com/scalar/db/storage/jdbc/JdbcAdmin.java Outdated Show resolved Hide resolved

KodaiD added 2 commits October 28, 2025 08:43

Disable the test for SQLite

ea08896

Add integration tests for ConsensusCommitAdmin

6d051e2

KodaiD marked this pull request as ready for review October 28, 2025 00:44

Copilot AI review requested due to automatic review settings October 28, 2025 00:44

Copilot AI reviewed Oct 28, 2025

View reviewed changes

Fix based on review

05b8493

KodaiD requested review from a team, Torch3333, brfrn169, feeblefakie and komamitsu and removed request for a team October 28, 2025 03:17

komamitsu approved these changes Oct 28, 2025

View reviewed changes

feeblefakie reviewed Oct 28, 2025

View reviewed changes

KodaiD added 3 commits October 28, 2025 17:54

Add an integration test case for all storages

96b13d3

Merge branch 'master' into fix-to-avoid-deleting-non-scalardb-tables-…

f03f72a

…when-dropping-namespaces

Fix

c68cf2a

KodaiD force-pushed the fix-to-avoid-deleting-non-scalardb-tables-when-dropping-namespaces branch from 209e173 to c68cf2a Compare October 29, 2025 00:15

KodaiD added 3 commits October 29, 2025 09:19

Resolve conflicts

c391eea

Add missing error

d34ea7a

Disable the test for DynamoDB

c90108b

KodaiD commented Oct 29, 2025

View reviewed changes

...gration-test/src/main/java/com/scalar/db/api/DistributedStorageAdminIntegrationTestBase.java Show resolved Hide resolved

KodaiD requested a review from feeblefakie October 29, 2025 01:39

brfrn169 reviewed Oct 29, 2025

View reviewed changes

KodaiD added 3 commits October 29, 2025 23:46

Fix based on review

a2f64c1

Resolve conflicts

b8bc276

Remove unnecessary override

6626c93

KodaiD requested a review from brfrn169 October 29, 2025 15:29

brfrn169 approved these changes Oct 30, 2025

View reviewed changes

core/src/main/java/com/scalar/db/storage/cosmos/CosmosAdmin.java Outdated Show resolved Hide resolved

core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java Show resolved Hide resolved

Apply suggestions

539bfa8

brfrn169 approved these changes Oct 30, 2025

View reviewed changes

feeblefakie reviewed Oct 30, 2025

View reviewed changes

KodaiD requested a review from feeblefakie October 30, 2025 05:01

Torch3333 approved these changes Oct 30, 2025

View reviewed changes

feeblefakie approved these changes Oct 30, 2025

View reviewed changes

brfrn169 merged commit 024172e into master Oct 30, 2025
127 of 128 checks passed

brfrn169 deleted the fix-to-avoid-deleting-non-scalardb-tables-when-dropping-namespaces branch October 30, 2025 07:39

Fix to avoid deleting non ScalarDB tables when dropping namespaces #3092

Fix to avoid deleting non ScalarDB tables when dropping namespaces #3092

Uh oh!

Conversation

KodaiD commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues and/or PRs

Changes made

Checklist

Additional notes (optional)

Release notes

Uh oh!

KodaiD commented Oct 27, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

komamitsu left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KodaiD commented Oct 28, 2025

Uh oh!

KodaiD commented Oct 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

KodaiD commented Oct 29, 2025

Uh oh!

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

brfrn169 left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

KodaiD commented Oct 30, 2025

Uh oh!

Torch3333 left a comment

Choose a reason for hiding this comment

Uh oh!

feeblefakie commented Oct 30, 2025

Uh oh!

feeblefakie left a comment

Choose a reason for hiding this comment

Uh oh!

KodaiD commented Oct 27, 2025 •

edited

Loading

feeblefakie left a comment •

edited

Loading

KodaiD commented Oct 29, 2025 •

edited

Loading